Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Expose Uplift theme #1640

Merged
merged 2 commits into from
Mar 19, 2024
Merged

fix: Expose Uplift theme #1640

merged 2 commits into from
Mar 19, 2024

Conversation

maael
Copy link
Contributor

@maael maael commented Mar 19, 2024

What does this implement/fix?

We need to be able to have two variations of the Uplift theme: a standard and a "ghost" version.

As such we can't simply overwrite properties of the theme as shown here.

Instead we need to define multiple themes as shown here.

To do this with Uplift as a base however, the theme needs to be exposed, which it currently isn't.

In theory this can be retrieved via the @shopify/polaris-viz-core, but the same is true for the other themes and that would mean that dependency becomes a direct dependency of the project, rather than through @shopify/polaris-viz.

Instead, it's proposed here that we just expose the Uplift theme in a manner similar to the others.

Does this close any currently open issues?

Related:

What do the changes look like?

There are no visual changes, this is a package API change.

Storybook link

There are no visual changes, this is a package API change.

Before merging

  • Check your changes on a variety of browsers and devices.

  • Update the Changelog's Unreleased section with your changes.

  • Update relevant documentation, tests, and Storybook.

  • Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package

maael added 2 commits March 19, 2024 09:56
We need to be able to have two variations of the Uplift theme: a
standard and a "ghost" version.

As such we can't simply overwrite properties of the theme as shown
[here](https://polaris-viz.shopify.com/iframe.html?viewMode=docs&id=shared-themes-customizing--page#titleAnchor-81).

Instead we need to define multiple themes as shown
[here](https://polaris-viz.shopify.com/iframe.html?viewMode=docs&id=shared-themes-customizing--page#titleAnchor-86).

To do this with Uplift as a base however, the theme needs to be exposed,
which it currently isn't.

In theory this can be retrieved via the `@shopify/polaris-viz-core`, but
the same is true for the other themes and that would mean that
dependency becomes a direct dependency of the project, rather than
through `@shopify/polaris-viz`.

Instead, it's proposed here that we just expose the Uplift theme in a
manner similar to the others.
@maael maael self-assigned this Mar 19, 2024
Copy link

github-actions bot commented Mar 19, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.36 KB (0%) 1.3 s (0%) 826 ms (-19.56% 🔽) 2.1 s
polaris-viz-cjs 215.52 KB (+0.01% 🔺) 4.4 s (+0.01% 🔺) 2 s (+13.79% 🔺) 6.3 s
polaris-viz-esm 175.93 KB (+0.01% 🔺) 3.6 s (+0.01% 🔺) 1.2 s (-12.23% 🔽) 4.7 s
polaris-viz-css 4.78 KB (0%) 96 ms (0%) 198 ms (-45.1% 🔽) 293 ms
polaris-viz-esnext 181.83 KB (+0.01% 🔺) 3.7 s (+0.01% 🔺) 1.2 s (-27.15% 🔽) 4.9 s

@maael maael requested review from a team and envex and removed request for a team March 19, 2024 10:07
@maael maael added the themes label Mar 19, 2024
@maael maael merged commit afcb282 into main Mar 19, 2024
6 checks passed
@maael maael deleted the fix/expose-uplift-theme branch March 19, 2024 14:02
@maael
Copy link
Contributor Author

maael commented Mar 19, 2024

@envex thanks for the review!

How are the releases for this package done please? There is some blocking feedback on a project about styling issues related to this, so having a version available with this soon would be appreciated.

@envex
Copy link
Collaborator

envex commented Mar 19, 2024

@maael I'll cut a release for you right now.

@maael
Copy link
Contributor Author

maael commented Mar 19, 2024

@envex 🙌 fantastic, thank you very much.

@envex
Copy link
Collaborator

envex commented Mar 19, 2024

@maael @shopify/[email protected] should have your changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants